Surface engine cooldown and classify network-level blocks on /monitor#1
Conversation
The /monitor page showed Yahoo as "healthy" while it was being blocked, and gave no indication of which engines the scraper had benched. Two gaps, both fixed here. Recognize network-level blocks: classify_engine and the scraper's cooldown tracker only treated HTTP 429/403/503 as a block, but Yahoo blocks at the TCP layer (net::ERR_CONNECTION_CLOSED) with no HTTP status, so it slipped through as healthy/degraded and was never benched. A new common/block_signals.is_network_block recognizes connection-level teardowns (not plain timeouts); it feeds both the cooldown tracker (Yahoo now gets benched instead of eating a nav timeout every cycle) and classify_engine (labelled "blocked" despite a NULL status). The metrics aggregate now carries last_error_message so the API can see it. Surface cooldown state: the EngineCooldownTracker lives in the scraper process and the API couldn't see it, so there was no "cooling down" concept at all. The scraper now snapshots tracker state to a new engine_cooldowns table each cycle (next_probe_at as absolute UTC, since the monotonic clock can't cross processes); /metrics overlays a "cooldown" health label with a probe countdown and synthesizes rows for benched engines that produced no logs, so they stay visible instead of vanishing. A stale snapshot (scraper down) is ignored. Docs (README, OBSERVABILITY, API_REFERENCE, BLOCK_SIGNAL_*) updated.
Reviewer's GuideAdds shared detection of network-level block signals and wires it into both the scraper and /metrics so Yahoo-style ERR_CONNECTION_CLOSED blocks are benched and shown as Sequence diagram for metrics request with network blocks and cooldown overlaysequenceDiagram
actor User
participant Monitor as MonitorApp
participant API as get_metrics
participant DB as database
User ->> Monitor: Load /monitor
Monitor ->> API: GET /api/v1/metrics
API ->> DB: get_scrape_metrics(window)
DB -->> API: overall, engines (with last_error_message)
API ->> DB: get_engine_cooldowns()
DB -->> API: engine_cooldowns
loop per engine row
API ->> API: health = classify_engine(row)
API ->> API: is_network_block(row.last_error_message)
API ->> API: remaining = _live_cooldown_seconds(cooldown, now)
API ->> API: engine_metrics = _build_engine(row, cooldown)
end
API -->> Monitor: MetricsResponse.engines[*]<br/>health (incl. blocked/cooldown)<br/>cooldown_seconds_remaining<br/>cooldown_failures
Monitor ->> Monitor: render health label and countdown
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
upsert_engine_cooldowns, theCASE WHEN %s > 0currently uses thefailuresvalue ((e, f, f, max(0.0, r))), sonext_probe_atis controlled by failures rather thanremaining_seconds; this should likely testremaining_secondsinstead so an engine with nonzero failures but zero remaining does not get an unnecessary future probe time. - The cooldown freshness/remaining logic mixes DB timestamps with
_naive_utc_now(); sinceengine_cooldowns.next_probe_at/updated_atareTIMESTAMPwithout time zone andNOW()uses the DB server clock, it would be safer either to store these astimestamptzor to be explicit about assuming the DB runs in UTC to avoid subtle drift between API and DB clocks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `upsert_engine_cooldowns`, the `CASE WHEN %s > 0` currently uses the `failures` value (`(e, f, f, max(0.0, r))`), so `next_probe_at` is controlled by failures rather than `remaining_seconds`; this should likely test `remaining_seconds` instead so an engine with nonzero failures but zero remaining does not get an unnecessary future probe time.
- The cooldown freshness/remaining logic mixes DB timestamps with `_naive_utc_now()`; since `engine_cooldowns.next_probe_at`/`updated_at` are `TIMESTAMP` without time zone and `NOW()` uses the DB server clock, it would be safer either to store these as `timestamptz` or to be explicit about assuming the DB runs in UTC to avoid subtle drift between API and DB clocks.
## Individual Comments
### Comment 1
<location path="common/database.py" line_range="684-693" />
<code_context>
+def upsert_engine_cooldowns(
</code_context>
<issue_to_address>
**issue (bug_risk):** Cooldown persistence uses `failures` instead of `remaining` to gate `next_probe_at`, which can keep already-expired cooldowns alive.
The `executemany` call currently passes `(e, f, f, max(0.0, r))`, so the `CASE WHEN %s > 0` is checking `failures` instead of `remaining_seconds`. For engines with `failures > 0` but `remaining_seconds <= 0`, this still sets `next_probe_at` to `NOW()`, keeping them incorrectly benched.
If the goal is for `next_probe_at` to be NULL whenever `remaining_seconds <= 0`, and only set in the future when `remaining_seconds > 0`, the CASE should use `remaining_seconds`, e.g.:
```python
a = [(e, f, max(0.0, r), max(0.0, r)) for (e, f, r) in states]
cursor.executemany(
"INSERT INTO engine_cooldowns (engine, failures, next_probe_at, updated_at) "
"VALUES (%s, %s, "
" CASE WHEN %s > 0 THEN NOW() + make_interval(secs => %s) END, "
" NOW()) "
"ON CONFLICT (engine) DO UPDATE SET "
" failures = EXCLUDED.failures, "
" next_probe_at = EXCLUDED.next_probe_at, "
" updated_at = EXCLUDED.updated_at",
a,
)
```
This ensures already-elapsed cooldowns are stored with `next_probe_at = NULL` and not treated as still benched.
</issue_to_address>
### Comment 2
<location path="api/static/monitor.js" line_range="114-122" />
<code_context>
row.querySelector('.c-health').dataset.health = e.health;
+ // Cooldown: show the countdown to the next probe beside the label.
+ const cd = row.querySelector('.ecooldown');
+ if (e.cooldown_seconds_remaining != null) {
+ cd.textContent = `~${this.fmtCountdown(e.cooldown_seconds_remaining)}`;
+ const blocks = e.cooldown_failures
+ ? `${e.cooldown_failures} consecutive block${e.cooldown_failures === 1 ? '' : 's'}; `
+ : '';
+ cd.title = `${blocks}probing again in ~${this.fmtCountdown(e.cooldown_seconds_remaining)}`;
+ } else {
+ cd.textContent = '';
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Cooldown tooltip (`title`) is never cleared when an engine stops cooling, leaving stale hover text.
In `updateEngineRow`, the `else` branch only clears `cd.textContent`, so when an engine stops cooling the previous `cd.title` remains and shows stale tooltip text. Clear the title as well:
```js
} else {
cd.textContent = '';
cd.title = '';
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def upsert_engine_cooldowns( | ||
| states: "list[tuple[str, int, float]]", | ||
| ) -> None: | ||
| """Persist the scraper's per-engine cooldown snapshot for the monitor. | ||
|
|
||
| ``states`` is ``(engine, failures, remaining_seconds)`` per tracked engine. | ||
| ``next_probe_at`` is stored as absolute UTC (``NOW() + remaining``) using the | ||
| *DB* clock so the API can derive a fresh countdown without trusting the | ||
| scraper's wall-clock; a non-cooling engine (failures == 0) stores NULL. | ||
| """ |
There was a problem hiding this comment.
issue (bug_risk): Cooldown persistence uses failures instead of remaining to gate next_probe_at, which can keep already-expired cooldowns alive.
The executemany call currently passes (e, f, f, max(0.0, r)), so the CASE WHEN %s > 0 is checking failures instead of remaining_seconds. For engines with failures > 0 but remaining_seconds <= 0, this still sets next_probe_at to NOW(), keeping them incorrectly benched.
If the goal is for next_probe_at to be NULL whenever remaining_seconds <= 0, and only set in the future when remaining_seconds > 0, the CASE should use remaining_seconds, e.g.:
a = [(e, f, max(0.0, r), max(0.0, r)) for (e, f, r) in states]
cursor.executemany(
"INSERT INTO engine_cooldowns (engine, failures, next_probe_at, updated_at) "
"VALUES (%s, %s, "
" CASE WHEN %s > 0 THEN NOW() + make_interval(secs => %s) END, "
" NOW()) "
"ON CONFLICT (engine) DO UPDATE SET "
" failures = EXCLUDED.failures, "
" next_probe_at = EXCLUDED.next_probe_at, "
" updated_at = EXCLUDED.updated_at",
a,
)This ensures already-elapsed cooldowns are stored with next_probe_at = NULL and not treated as still benched.
| const cd = row.querySelector('.ecooldown'); | ||
| if (e.cooldown_seconds_remaining != null) { | ||
| cd.textContent = `~${this.fmtCountdown(e.cooldown_seconds_remaining)}`; | ||
| const blocks = e.cooldown_failures | ||
| ? `${e.cooldown_failures} consecutive block${e.cooldown_failures === 1 ? '' : 's'}; ` | ||
| : ''; | ||
| cd.title = `${blocks}probing again in ~${this.fmtCountdown(e.cooldown_seconds_remaining)}`; | ||
| } else { | ||
| cd.textContent = ''; |
There was a problem hiding this comment.
issue (bug_risk): Cooldown tooltip (title) is never cleared when an engine stops cooling, leaving stale hover text.
In updateEngineRow, the else branch only clears cd.textContent, so when an engine stops cooling the previous cd.title remains and shows stale tooltip text. Clear the title as well:
} else {
cd.textContent = '';
cd.title = '';
}
The /monitor page showed Yahoo as "healthy" while it was being blocked, and gave no indication of which engines the scraper had benched. Two gaps, both fixed here.
Recognize network-level blocks: classify_engine and the scraper's cooldown tracker only treated HTTP 429/403/503 as a block, but Yahoo blocks at the TCP layer (net::ERR_CONNECTION_CLOSED) with no HTTP status, so it slipped through as healthy/degraded and was never benched. A new common/block_signals.is_network_block recognizes connection-level teardowns (not plain timeouts); it feeds both the cooldown tracker (Yahoo now gets benched instead of eating a nav timeout every cycle) and classify_engine (labelled "blocked" despite a NULL status). The metrics aggregate now carries last_error_message so the API can see it.
Surface cooldown state: the EngineCooldownTracker lives in the scraper process and the API couldn't see it, so there was no "cooling down" concept at all. The scraper now snapshots tracker state to a new engine_cooldowns table each cycle (next_probe_at as absolute UTC, since the monotonic clock can't cross processes); /metrics overlays a "cooldown" health label with a probe countdown and synthesizes rows for benched engines that produced no logs, so they stay visible instead of vanishing. A stale snapshot (scraper down) is ignored.
Docs (README, OBSERVABILITY, API_REFERENCE, BLOCK_SIGNAL_*) updated.
Summary by Sourcery
Recognize network-level connection teardowns as block signals and surface the scraper’s adaptive cooldown state on the /monitor metrics API and UI.
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: